Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add the call of DropKids at the end of executor.Run to delete the local scopes created in operators #10403

Merged
merged 3 commits into from
May 7, 2018
Merged

Add the call of DropKids at the end of executor.Run to delete the local scopes created in operators #10403

merged 3 commits into from
May 7, 2018

Conversation

Xreki
Copy link
Contributor

@Xreki Xreki commented May 4, 2018

No description provided.

@Xreki Xreki added the 预测 原名Inference，包含Capi预测问题等 label May 4, 2018
@Xreki Xreki requested a review from jacquesqiao May 4, 2018 09:29
@Xreki Xreki changed the title Add the call of DeleteScope to delete the memory of scope created by NewScope Add the call of DropKids at the end of executor.Run to delete the local scopes created in operators May 4, 2018
jacquesqiao
jacquesqiao reviewed May 4, 2018
View reviewed changes
paddle/fluid/framework/executor.cc
if (create_vars && create_local_scope) {
scope->DeleteScope(local_scope);
} else {
// Delete the local scopes created in operators.
scope->DropKids();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if create_vars is False and create_local_scope is True, do we need to use

local_scope->DropKids()

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The local_scope itself is deleted by scope->DeleteScope(local_scope);, all the kids will be deleted, see the implementation of the destructor:

Paddle/paddle/fluid/framework/scope.cc

Lines 37 to 43 in 0285a2b

Scope::~Scope() {
DropKids();
for (auto& kv : vars_) {
VLOG(3) << "Destroy variable " << kv.first;
delete kv.second;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Xreki , maybe we can't delete the kids in the execute function, some conditions like dist training, pserver will execute the optimize blocks on the same scope instead of a new scope for each optimizes action.

related issued #10625

jacquesqiao
jacquesqiao approved these changes May 4, 2018
View reviewed changes
Copy link
Member

@jacquesqiao jacquesqiao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Xreki Xreki merged commit fd1971c into PaddlePaddle:develop May 7, 2018
@reyoung reyoung mentioned this pull request Jun 20, 2018
Closed
@Xreki Xreki deleted the core_fix_delete_scope branch November 14, 2018 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yancey1989 Yancey1989 Yancey1989 left review comments

@jacquesqiao jacquesqiao jacquesqiao approved these changes

Assignees
No one assigned
Labels
预测 原名Inference，包含Capi预测问题等
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Xreki @Yancey1989 @jacquesqiao